perf: cut startup cost across natives, providers, plugins and MCP (W1-W6) - #3846
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38a28670d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function readBundledContentSync(entry: BundledGjcSkillCatalogEntry): string { | ||
| const sourcePath = sourcePathForBundledEntry(entry); | ||
| try { | ||
| return readFileSync(sourcePath, "utf8"); |
There was a problem hiding this comment.
Avoid fs reads for bundled skill content
When this runs from a compiled Bun executable, sourcePath resolves under /$bunfs/...; literal text imports are available there, but readFileSync/Bun.file against that path returns ENOENT. Because content on the exported bundled default definitions and embedded skills now calls this getter, any compiled-binary caller that still reads definition.content/skill.content instead of the new async loadContent() loses the bundled workflow skill body. Keep the text behind the generated import loaders/cache rather than reading the source path synchronously.
AGENTS.md reference: AGENTS.md:L112-L122
Useful? React with 👍 / 👎.
| } | ||
| if (isManagedOwnerSupervisorArgv(argv)) { | ||
| if (argv.length === 1 && argv[0] === MANAGED_OWNER_SUPERVISOR_ARG) { | ||
| const { runManagedOwnerSupervisor } = await import("./gjc-runtime/managed-owner-supervisor"); |
There was a problem hiding this comment.
Install the supervisor SIGTERM guard before lazy import
For the internal --internal-managed-owner-supervisor invocation, this lazy import leaves a startup window before managed-owner-supervisor.ts's module-scope SIGTERM capture is installed. If the pane close flow sends SIGTERM while the CLI is still loading or before this branch imports the module, the supervisor exits via the default handler instead of preserving bootstrapSigtermPending and relaying/observing the exact child termination, so managed-owner recovery loses its trusted terminal evidence. Keep a tiny early handler or load the supervisor guard before normal CLI initialization.
AGENTS.md reference: AGENTS.md:L101-L110
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38a28670d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| options.onNotification?.(method, params); | ||
| this.emit(entry, { type: "notification", method, params }); |
There was a problem hiding this comment.
Dispatch each pooled notification only once
When a shared MCP entry receives a notification, this calls the first acquirer's onNotification directly and then emits the same event to every lease, including that first lease. Its manager therefore processes tools/list_changed, resources/list_changed, and similar events twice, causing duplicate refresh requests and duplicate consumer callbacks whenever it owns the physical connection; route notifications exclusively through the lease fan-out.
Useful? React with 👍 / 👎.
| const plan = planToolOutputPrune(branchEntries, { | ||
| ...DEFAULT_PRUNE_CONFIG, | ||
| minimumSavings: overThreshold ? 0 : DEFAULT_PRUNE_CONFIG.minimumSavings, | ||
| }); | ||
| const artifactManager = this.sessionManager.getArtifactManager(); |
There was a problem hiding this comment.
Establish artifacts before pruning ephemeral sessions
For a non-persistent/SDK session with no session file and no previously saved artifact, getArtifactManager() returns null; consequently nothing is added to published, the committed plan is empty, and stale tool outputs are never pruned. Long ephemeral sessions therefore fall through to compaction or context overflow even though ensureArtifactManager() can create the supported temporary artifact store; establish that store before publishing prune candidates.
Useful? React with 👍 / 👎.
| this.#connectionDisposer?.(); | ||
| this.#capabilitiesDisposer?.(); | ||
| this.#malformedDisposer?.(); |
There was a problem hiding this comment.
Reinstall SDK transport handlers after restart
When the same SessionSdkSessionRuntime is stopped and then started again, these one-time disposers permanently remove the connection-close, capability-negotiation, and malformed-frame handlers, while start() never registers replacements. The restarted runtime consequently retains reverse leases after disconnects, ignores negotiated capabilities, and stops returning structured malformed-frame errors; either make the runtime explicitly terminal after stop or recreate these subscriptions on every start.
Useful? React with 👍 / 👎.
38a2867 to
2f2e32c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f2e32c536
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| !/\.(?:optional|nullable)\s*\(\s*\)\s*$/.test(field[2]!) && | ||
| !/\.Optional\s*\(\s*\)\s*$/.test(field[2]!) && | ||
| !/(?:Type\.)?Optional\s*\(/.test(field[2]!) |
There was a problem hiding this comment.
Keep nullable plugin parameters required
For plugin tools whose schema is inferred from source, a declaration such as z.string().nullable() is classified as optional here. Nullable fields may contain null, but their presence is still required, so the published schema permits the model to omit a value that the plugin's runtime validator rejects; the static expression parser also reduces this chained expression to the underlying string schema and therefore rejects the valid null value. Treat only optional wrappers as removing a property from required, while preserving nullability in the property schema.
Useful? React with 👍 / 👎.
| const runtime = await this.#workspaceTreeService.get("first-turn-barrier"); | ||
| this.#cachedWorkspaceTree = firstWorkspaceTree ? runtime.snapshot : await runtime.refresh(); |
There was a problem hiding this comment.
Preserve best-effort workspace scanning in lazy mode
With workspaceTree.mode="lazy", any native scan error or filesystem failure from the service now escapes this first-turn barrier and rejects prompt() instead of omitting the workspace tree as the fallback branch and the previous implementation do. Because LazyService retains the failed state, subsequent prompts keep rejecting as well, making an optional context scan permanently disable the session; catch scan/service failures here and continue with an undefined tree.
Useful? React with 👍 / 👎.
| const endpoint = await this.transport.start(); | ||
| this.#transportStarted = true; | ||
| return endpoint; |
There was a problem hiding this comment.
Prevent transport startup from winning after shutdown
If stop() runs while transport.start() is pending, the transport shutdown path waits for that start and then closes the newly created server, but this continuation still sets #transportStarted to true and returns its endpoint. The start caller can therefore receive a URL whose endpoint file/server has already been removed, while the runtime incorrectly reports the transport as started; coordinate start and stop with a generation or shutdown flag so an in-flight start cannot publish success after shutdown begins.
Useful? React with 👍 / 👎.
2f2e32c to
ad2110b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad2110b2fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const localBackend: MemoryBackend = { | ||
| id: "local", | ||
| async start(options) { | ||
| return (await import("./local-backend")).localBackend.start(options); |
There was a problem hiding this comment.
Replace inline backend imports with approved lazy loaders
Replace this await import() path, along with the other method-level imports in these backend facades, with the repository's approved top-level/static loading pattern. The repository contract explicitly prohibits inline imports, and these calls make the memory backend's dependency and compiled-binary loading behavior depend on runtime method execution rather than the statically inspectable module graph.
AGENTS.md reference: AGENTS.md:L103-L106
Useful? React with 👍 / 👎.
| const plan = planToolOutputPrune(branchEntries, { | ||
| ...DEFAULT_PRUNE_CONFIG, | ||
| minimumSavings: overThreshold ? 0 : DEFAULT_PRUNE_CONFIG.minimumSavings, | ||
| }); |
There was a problem hiding this comment.
Account for artifact references when admitting a prune
When below-threshold pruning is near minimumSavings, this plan is built without artifactRefMaxChars, so admission assumes the replacement has no artifact URI; the method then appends the published URI and can commit even after actual savings fall below the configured minimum, because the optional commit gate checks only cache-reset cost. This can trigger a history rewrite and provider-cache reset for a prune that the documented minimum-savings gate should have rejected; build the committed plan with the same artifact-reference budget used by the preflight estimate or recheck the final savings against the minimum.
Useful? React with 👍 / 👎.
ad2110b to
5933677
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5933677db4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| active = { runtime, revisions, cursors, reconciliation, pending, disposeGate }; | ||
| try { | ||
| await runtime.start(); |
There was a problem hiding this comment.
Register SDK-only sessions with the broker
When notifications are disabled—the default for ordinary top-level sessions—createAgentSession selects this SDK-only extension, but startup ends after runtime.start() and never calls the available runtime.registerWithBroker(...) path. Unlike the previous notification-host path, no host_registered record reaches SessionIndex, so broker-backed session.list, SDK attach, and relay commands cannot discover these otherwise-live endpoints.
Useful? React with 👍 / 👎.
| await filesystem.writeFile( | ||
| endpointFile, | ||
| JSON.stringify({ version: 1, url, token: input.token, pid: process.pid }), | ||
| "utf8", |
There was a problem hiding this comment.
Include the session identity in SDK endpoint records
For sessions using the new WebSocket transport, this endpoint record omits sessionId. Broker endpoint resolution explicitly rejects records unless endpoint.sessionId === record.sessionId (sdk/broker/broker.ts:784-791), and lifecycle readiness applies the same check, so even after the SDK-only host is indexed, broker attach/readiness treats its endpoint as stale. Publish input.sessionId in the discovery JSON, matching the existing endpoint contract.
Useful? React with 👍 / 👎.
| const bindings = nativeThemeBindings; | ||
| loadNativeThemeBindings(); | ||
| const validLang = bindings && lang && bindings.supportsLanguage(lang) ? lang : undefined; | ||
| if (!bindings) return code.split("\n").map(line => theme.fg("mdCodeBlock", line)); |
There was a problem hiding this comment.
Load syntax bindings before one-shot rendering
On non-macOS hosts and in non-interactive/print mode, theme initialization does not preload the native bindings. The first code block therefore captures undefined, starts an asynchronous load, and immediately returns unhighlighted text; print mode has no subsequent render, so syntax highlighting remains absent despite syntaxHighlighting.enabled defaulting to true. Await the binding load before one-shot rendering or trigger a rerender when it completes.
Useful? React with 👍 / 👎.
경고 — 이 PR의 현재 head가 CHANGELOG 전체를 삭제한다머지하면 안 된다. 확인된 사실: 1바이트 — 개행 하나만 남았다. 원인은 내 쪽이다#3932(11:25:32Z 머지)가 그런데 그 결과 리베이스에서 CHANGELOG가 처음으로 진짜 충돌을 내기 시작했고, 그 충돌을 해소하는 과정에서 파일이 비워졌다. 시간대가 명확하다:
전환 비용을 예고하지 못한 건 내 잘못이다. 미안하다. 복구git fetch origin
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md # 해당 패키지 경로로
# 그 다음 ## [Unreleased] 아래에 이 PR의 항목만 다시 추가
git add packages/coding-agent/CHANGELOG.md
git commit --amend --no-edit # 또는 새 커밋앞으로 리베이스에서 CHANGELOG 충돌이 나면 양쪽 항목을 모두 푸시 전에 다음으로 자가 점검할 수 있다: git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # 30만 바이트 근처여야 정상 |
5933677 to
3b5de5c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b5de5cbe7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (range !== null) { | ||
| content = await file.slice(start, boundedEnd).text(); |
There was a problem hiding this comment.
Cap explicit artifact ranges before reading
For an artifact larger than 16 MiB, an explicit open-ended or wide query such as ?range=0- enters this branch and materializes the entire file, completely bypassing the ceiling applied to bare reads. Large spilled artifacts can therefore allocate hundreds of MiB or more and crash the session; clamp every requested range to a bounded maximum before calling text().
Useful? React with 👍 / 👎.
| const stopResult = current.stop(true); | ||
| await Promise.race([stopResult, new Promise<void>(resolve => setTimeout(resolve, 250))]); |
There was a problem hiding this comment.
Cancel the SDK shutdown timeout after a fast stop
When current.stop(true) resolves before 250 ms, Promise.race drops the timeout promise but does not cancel its timer. Bun keeps that timer alive, so an otherwise-complete SDK shutdown can hold process exit open for the remainder of the 250 ms window; use a cancellable timeout and clear it when the stop settles.
AGENTS.md reference: AGENTS.md:L108-L109
Useful? React with 👍 / 👎.
| void this.#ensureHistoryStorage().then(storage => { | ||
| if (storage && this.#history.length > 0) this.#navigateHistory(direction); | ||
| this.invalidate(); |
There was a problem hiding this comment.
Discard stale asynchronous history navigation
When history has not loaded yet, pressing Up starts this asynchronous load; if the user types or otherwise changes the draft before it resolves, the continuation still calls #navigateHistory(direction) and replaces the newer draft with an old prompt. Capture the editor/document state at the initial keypress and only navigate if it is still unchanged when loading completes.
Useful? React with 👍 / 👎.
3b5de5c to
9549e2f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9549e2fe72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const maxDepth = session.settings.get("task.maxRecursionDepth") ?? 2; | ||
| return maxDepth < 0 || (session.taskDepth ?? 0) < maxDepth; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Exclude unavailable Telegram tools from discovery
When tools.discoveryMode uses its default all value and Telegram notifications are disabled, telegram_send falls through to this unconditional true, so createTools() publishes a deferred LazyAgentTool without running TelegramSendTool.createIf(). The model can then discover and activate the tool, but its first execution loads a null implementation and fails with the generic “failed to load” error. Mirror the provider-enabled predicate from tools/telegram-send.ts in descriptor availability so the discovery catalog does not advertise an unavailable capability.
Useful? React with 👍 / 👎.
PR #3846 altered protected Telegram lifecycle symbols (defaultProcessReference, exactUnlinkNotificationFile, process-incarnation authority). The daemon generation guard requires a strict generation bump for these changes. Generation 54 covers the startup-cost refactoring that moved these symbols off the eager import path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 099918b3cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "initTheme:final", | ||
| deps.initTheme ?? initTheme, | ||
| isInteractive, | ||
| isInteractive && settingsInstance.get("theme.watchFiles"), |
There was a problem hiding this comment.
Decouple theme file watching from appearance detection
When an interactive user sets theme.watchFiles=false, this passes false as initTheme's general enableWatcher argument. That argument also gates startSigwinchListener() and the macOS appearance observer in theme.ts, so disabling custom-theme file reloads unexpectedly disables automatic dark/light theme detection and subsequent terminal appearance updates. Keep appearance monitoring enabled for interactive sessions and gate only the file watcher with this setting.
Useful? React with 👍 / 👎.
099918b to
b750886
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b750886fd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.#sleep = options.sleep ?? delay; | ||
| this.#afterLeaseAcquiredForTests = options.afterLeaseAcquiredForTests; | ||
| this.cwd = canonicalMCPWorkingDirectory(this.cwd); | ||
| this.#pool = options.pool ?? new MCPConnectionPool({ sharedPoolIdleMs: options.sharedPoolIdleMs }); |
There was a problem hiding this comment.
Reuse one pool across MCP managers
When multiple MCP managers in the same process connect to a server configured with sharing: "shared", each manager still creates its own MCPConnectionPool, so their keys and leases can never meet and each facade opens a separate physical connection. I checked the production constructors in sdk/session.ts, the MCP loader, and runtime command controllers; they omit options.pool, while only tests inject a common pool. Use a process/session-host-owned shared pool for managers that are expected to honor shared mode.
Useful? React with 👍 / 👎.
| if (this.historyStorage) return this.historyStorage; | ||
| if (this.settings.get("history.enabled") === false) return undefined; |
There was a problem hiding this comment.
Honor history disablement after storage loads
If history is initially enabled and the editor loads HistoryStorage, changing history.enabled to false later has no effect because this cached-storage return bypasses the setting check; subsequent prompts continue to be written even though history search is disabled. The same issue occurs when the setting is turned off while openAsync() is pending, because its continuation still installs the storage. Recheck the live setting before returning/installing storage and detach persistence when the setting is disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f8e8d1581
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const loaders: Record<string, Loader> = { | ||
| read: session => cached("read", () => import("./read")).then(module => new module.ReadTool(session)), |
There was a problem hiding this comment.
Replace inline imports in the built-in tool loader
This new loader table resolves every built-in through runtime import() calls, despite the repository contract requiring top-level imports only. Besides making dependencies invisible to the static module graph, this leaves compiled-binary loading behavior dependent on runtime activation; use the repository's approved statically imported lazy-loader pattern instead.
AGENTS.md reference: AGENTS.md:L103-L106
Useful? React with 👍 / 👎.
| } | ||
| if (requireDescription && !frontmatter.description) { | ||
| const frontmatter = await readSkillFrontmatter(skillPath); | ||
| if (!frontmatter) { |
There was a problem hiding this comment.
Preserve frontmatter-free skills during lazy scanning
When requireDescription is false, a SKILL.md without YAML frontmatter was previously loaded using its directory name and full body, but readSkillFrontmatter() returns null for that valid input and this branch now drops it unconditionally. The Codex, OpenCode, Claude, and agents discovery paths call this scanner without requiring a description, so existing frontmatter-free custom skills silently disappear from discovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba7002b832
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bisect: session => cached("bisect", () => import("./bisect")).then(module => new module.BisectTool(session)), | ||
| eval: session => cached("eval", () => import("./eval")).then(module => new module.EvalTool(session)), | ||
| calc: session => cached("calc", () => import("./calculator")).then(module => new module.CalculatorTool(session)), | ||
| ssh: session => cached("ssh", () => import("./ssh")).then(module => module.loadSshTool(session)), |
There was a problem hiding this comment.
Hide SSH until a host configuration exists
With the default tools.discoveryMode="all", this discoverable loader publishes ssh even when the session has no configured SSH hosts. Activating it then makes loadSshTool() return null, which LazyAgentTool surfaces as the generic Tool "ssh" failed to load error; the previous eager factory path omitted the tool in this case. Check host availability before adding the deferred descriptor, or keep SSH eager until hosts are known.
Useful? React with 👍 / 👎.
| monitor: session => | ||
| cached("monitor", () => import("./monitor")).then(module => module.MonitorTool.createIf(session)), | ||
| cron: session => cached("cron", () => import("./cron")).then(module => module.CronTool.createIf(session)), | ||
| recipe: session => cached("recipe", () => import("./recipe")).then(module => module.RecipeTool.createIf(session)), |
There was a problem hiding this comment.
Hide recipes when no runnable tasks exist
Because recipe discovery and recipe.enabled both default on, repositories without a supported runner containing tasks still advertise this deferred tool. On activation, RecipeTool.createIf() detects zero runnable tasks and returns null, so the model receives a generic failed-to-load error for a capability that should not have been discoverable; preserve the factory's task-detection guard before publishing the descriptor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c77b91fa42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| export class LazyServiceReentrantDisposeError extends Error { | ||
| readonly id: string; |
There was a problem hiding this comment.
Declare the lazy initialization result type explicitly
Replace this ReturnType<>-derived alias with an explicit initialization-result interface. The repository contract prohibits ReturnType<>, and keeping the result shape explicit prevents this service's internal state type from changing implicitly whenever the initializer callback signature is edited.
AGENTS.md reference: AGENTS.md:L104-L104
Useful? React with 👍 / 👎.
| return lease; | ||
| } | ||
|
|
||
| private waitForPendingEntry( |
There was a problem hiding this comment.
Convert the pool helpers to ES private methods
Convert this and the other newly introduced private pool helpers to #waitForPendingEntry, #closeEntry, and so on. The repository requires ES #private fields rather than TypeScript access modifiers, so the current pool implementation violates the established runtime-private convention throughout the class.
AGENTS.md reference: AGENTS.md:L108-L108
Useful? React with 👍 / 👎.
c77b91f to
0ac498f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ac498f7dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| required.push(key); | ||
| } | ||
| return { type: "object", properties, ...(required.length > 0 ? { required } : {}), additionalProperties: true }; |
There was a problem hiding this comment.
Preserve strictness in inferred plugin schemas
When a source-inferred plugin declares parameters: z.object({...}).strict(), callName() parses only the initial object() call and this emits additionalProperties: true, ignoring the trailing strict wrapper. The registry consequently advertises extra keys as valid even though the loaded Zod schema rejects them at execution time, so model-generated calls that conform to the published schema can fail validation; preserve supported trailing constraints or reject unsupported chains instead.
Useful? React with 👍 / 👎.
| await filesystem.writeFile( | ||
| endpointFile, | ||
| JSON.stringify({ version: 1, url, token: input.token, pid: process.pid }), | ||
| "utf8", | ||
| ); |
There was a problem hiding this comment.
Publish SDK endpoints without following existing symlinks
When a resumed session's known endpoint filename has been pre-created as a symlink, this direct writeFile follows it and truncates a file outside .gjc/state; the subsequent chmod also operates on that target. Because the endpoint path is workspace-local, stale or hostile workspace state can therefore corrupt arbitrary user files during SDK startup; publish a mode-0600 temporary file and atomically rename it over the endpoint, as the other SDK discovery writers do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6014c24a07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.#powerAssertionLoad = Promise.resolve() | ||
| .then(() => { |
There was a problem hiding this comment.
Cancel pending macOS assertion acquisition on release
When a turn exits synchronously after #beginInFlight()—for example, the preflight cancellation or missing-model checks can throw before the first await—#releasePowerAssertion() runs while #powerAssertion is still unset, and this queued continuation subsequently starts the assertion anyway. The orphaned assertion can prevent system sleep until another turn finishes or the session is disposed; keep acquisition synchronous or track cancellation/release state before calling MacOSPowerAssertion.start().
AGENTS.md reference: AGENTS.md:L105-L105
Useful? React with 👍 / 👎.
| const known = new Set(existing.map(entry => path.resolve(entry.pluginRoot))); | ||
| const discovered: GjcPluginRegistryEntry[] = []; | ||
| for (const dirent of dirents) { | ||
| if (!dirent.isDirectory() || dirent.name.startsWith(".")) continue; |
There was a problem hiding this comment.
Preserve symlinked plugins during legacy discovery
When an existing user or project plugin is installed as a symlink and has not yet been indexed in registry.json, this filter skips it because Dirent.isDirectory() is false for symlinks. The previous discovery path explicitly accepted isSymbolicLink(), so these valid legacy plugins now silently disappear instead of being migrated and activated; retain symlink candidates and let the compiler's containment and hash validation decide whether they are safe.
Useful? React with 👍 / 👎.
…alidation The legacy batch adapter had drifted from the native path: thrown steps lost their failureCode and failureIndex, timeoutMs was ignored so a non-terminating action ran unbounded, and batch dispatch reached the native seam without the pre-dispatch bounds check that the single-action path applies. A batch could therefore act on coordinates the single path would reject. Lore-id: 3fb6c082 Constraint: batch and single dispatch must agree on failure shape, deadline and coordinate validation Rejected: treating the divergence as legacy-path tolerance | it silently widened what a batch may do Confidence: high Scope-risk: narrow Reversibility: easy Tested: batch step failure mapping, deadline cancellation, and COMPUTER_COORD_INVALID on the batch path; the seven-case enforcement suite Not-tested: real display hardware -- the controller is driven through its test seam
The tool index imported every implementation to expose descriptors, so listing tools pulled their whole dependency set. Descriptors now live apart from implementations behind a generated catalog, letting discovery read metadata without constructing anything. Lore-id: 5d92ae3b Constraint: the generated catalog must stay in sync with its generator -- the literal-catalog trace gate enforces it Confidence: high Scope-risk: moderate Reversibility: easy Directive: change the generator, then regenerate; do not hand-edit tool-catalog.generated.ts Tested: literal catalog gates for tools and skills; tool-discovery initial-tools suite Not-tested: nothing material beyond the catalog gates
Session construction eagerly built artifact and history storage even for runs that never touched either. Both are now created on first use, which keeps their dependencies off the startup path. Lore-id: c6e14b09 Constraint: storage identity must stay stable across the deferral -- a session that later writes must land in the same place it would have Confidence: high Scope-risk: moderate Reversibility: easy Tested: agent-session suites covering concurrency, handoff, mid-run maintenance, detached bash, todos and replay Not-tested: resume from sessions written by older builds
…p path Follow-through for the laziness milestones across the surfaces the earlier commits exposed: MCP capability and CLI entry points, skill discovery and defaults, eval executors, and the settings and schema surfaces that describe them. Each moves construction behind first use or narrows an import so the startup graph stops paying for work a given invocation never performs. Lore-id: 0b7fd253 Constraint: compatibility defaults must not shift -- workspaceTree.mode stays eager and startup.networkPrewarm stays true in this change Rejected: flipping the defaults here to bank the win | that is a separate authorized milestone and would change behavior without review Confidence: medium Scope-risk: wide Reversibility: reversible Tested: coding-agent source suites; help full-deny, help+idle bun:sqlite and provider deny, and literal catalog trace gates; check:schemas Not-tested: every downstream consumer of the touched settings shapes
…ubpath Two boundaries needed manifest and lint enforcement rather than convention. Biome now rejects bare @gajae-code/ai imports inside packages/coding-agent/src, and the coding-agent export map null-blocks the legacy GJC plugin loader subpath -- both spellings, ordered before the recursive ./extensibility/* wildcard that would otherwise resolve it. packages/ai carries require conditions on ./* and ./providers/* so subpath requires resolve under bun --compile. Lore-id: e94a5b71 Constraint: null export entries must precede the wildcard; after it they are dead Constraint: the root barrel stays intact for external consumers Rejected: relying on review to catch bare @gajae-code/ai imports | the graph regression is invisible without a gate Confidence: high Scope-risk: moderate Reversibility: easy Tested: child-process package-subpath resolution failure for both loader spellings; packed SDK smoke at root 381 / sdk 39; restricted-import lint Not-tested: consumers already importing the legacy loader subpath -- they break by design
The laziness work needed measurable gates rather than reviewer judgement. verify-module-trace asserts which modules a scenario may load and pins the literal tool and skill catalogs; verify-rss-checkpoints measures whole-process-tree RSS at an explicit barrier and compares against a per-commit baseline with floor enforcement and schema-bound re-scope records. Omitting --baseline with --compare resolves only the canonical .gjc/rss-checkpoints/<commit>.json and otherwise fails closed with a typed BaselineDefaultMissing naming that path. S6 is emitted as deferred with its authorization reason instead of being measured or fabricated. Lore-id: b30c8fd4 Constraint: never fall back to <commit>.last-run.json -- a non-baseline run overwrites it, so comparing against it makes the gate oscillate between pass and fail on identical invocations Constraint: no implicit --allow-baseline-drift; drift stays an explicit opt-in Rejected: same-commit last-run fallback | observed flipping exit 0 and FAIL across back-to-back identical runs Rejected: fabricating an S6 measurement to fill the table | the scenario needs a daemon that does not exist yet Confidence: high Scope-risk: moderate Reversibility: easy Directive: stable-tree RSS at the barrier is the deciding metric; sampled peak stays diagnostic Tested: harness-gates covering canonical resolution, refusal of same-commit and foreign last-run fallbacks, malformed baselines, deferred S6 shape, and explicit --baseline precedence Not-tested: CI hardware -- S1 and S7 vary widely under load on this host
The session-runtime extraction also flipped the broker's no-cleanup session.delete fallback from an idempotent ok to invalid_input, which breaks the manifest-pinned adapter disposition contract (AD-*-G07) for every machine adapter. Deleting an unknown session stays a successful no-op. Lore-id: 7c41a9e2 Confidence: high Scope-risk: narrow Reversibility: reversible Tested: bun test packages/coding-agent/test/sdk-adapter-dispositions.test.ts (576 pass)
src/tools/tool-catalog.test.ts imported ../../scripts/generate-tool-catalog, so the publish type check emitted a stray scripts/generate-tool-catalog.d.ts into the package and the next `biome check .` failed on it. The test belongs with the other tool tests, where importing scripts/ is already normal. Lore-id: b83d5510 Confidence: high Scope-risk: narrow Reversibility: reversible Tested: bun test packages/coding-agent/test/tools/tool-catalog.test.ts (3 pass)
chat-daemon-control.ts moved its native process and unlink access behind lazy bindings, so the semantic declaration digests the guard pins had to be regenerated with --write-manifest. Lore-id: 5a2f77c1 Confidence: high Scope-risk: narrow Reversibility: reversible Tested: bun scripts/telegram-daemon-generation-guard.ts --validate-current-tree
`biome check .` was clean on dev and reported 225 errors on this branch: unformatted files, unsorted imports, and refactor residue — dead `import type * as native` aliases, an unused DiffQueryError copy in sdk/bus, an orphaned #ensureDir, unused native type aliases, and a cancelPendingEntry parameter no caller needs. findParametersExpression kept a while-assign loop that could only ever run once, and safeIsInstanceOf shadowed the global `constructor`. Lore-id: 9d2c04b7 Confidence: high Scope-risk: wide Reversibility: reversible Tested: biome check . (clean) Not-tested: no behavior change intended beyond the dead-code removals
Deferring @gajae-code/natives behind an async accessor added a microtask yield inside startSession before it registers in sessionStartPromises, so two concurrent `/notify on` calls each built a runtime and the loser threw "Lifecycle SDK startup was cancelled". require() is synchronous, so the accessor does not need to be async to stay lazy. Lore-id: 3e6fb18d Constraint: the native must stay off the startup module graph -- keep the in-function require Confidence: high Scope-risk: narrow Reversibility: reversible Tested: bun test packages/coding-agent/test/sdk-host-wiring.test.ts (78 pass)
Deferring discoverable tools advertises them from the descriptor alone, so every factory guard that used to drop a tool at creation had to move into availableFor. Without it a headless session advertised `ask` and only failed at call time; the same held for `checkpoint`/`rewind` in subagents, `irc` without an agent registry, `github` without the gh CLI, and `cron` under CLAUDE_CODE_DISABLE_CRON. fetch's html-to-markdown accessor cached the bound export rather than the module, freezing the first-seen implementation for the process. Lore-id: 1f7ad64c Constraint: availability must stay cheap -- no heavy imports on the descriptor path Rejected: eager materialization for conditional tools | reloads exactly what the deferral removed Confidence: high Scope-risk: medium Reversibility: reversible Tested: bun test packages/coding-agent/test/tools packages/coding-agent/src/tools (1719 pass) Not-tested: telegram_send availability still resolves at load; its guard needs the notification snapshot
The self-test expected moving continueStalledGjcTeamWorkers after stale-claim reconciliation to fail, but exactTeamRuntimeSendKeysRanges validated only the continuation function and ignored its monitor call site. Pin one continuation call before one reconciliation call inside monitorGjcTeam, and keep the direct Bun.spawnSync adversarial fixture syntactically valid. Lore-id: c61a9df4 Confidence: high Scope-risk: narrow Reversibility: reversible Tested: bun packages/coding-agent/scripts/verify-gjc-sdk-canonicalization.ts --self-test
The PR changes public package surfaces and runtime behavior across the AI, agent, coding-agent, TUI, and utils workspaces. Record the core entrypoint, telemetry fast path, lazy native loading, MCP/plugin/runtime changes, and the correctness fixes under each package's Unreleased section. Lore-id: 6f7430ad Confidence: high Scope-risk: narrow Reversibility: reversible Tested: git diff --check
The RSS harness test read three ignored `.gjc/rss-checkpoints` files that existed only in the author's worktree, so CI could never run M6. Commit the minimal immutable W1c identity evidence under scripts/fixtures and read it there instead. The descriptor availability matrix also assumed a Darwin-arm64 computer backend and an unset cron-disable variable. Derive those expected exclusions from the same platform and environment predicates as the descriptor. Lore-id: e3f8256a Confidence: high Scope-risk: narrow Reversibility: reversible Tested: CLAUDE_CODE_DISABLE_CRON=1 bun test packages/coding-agent/src/tools/descriptors.test.ts Tested: bun test scripts/harness-gates.test.ts
Deferred modules changed first-use timing and captured several exports too early, breaking syntax highlighting, clipboard spies, memory startup joins, pruning, MCP leases, broker validation, and test-only inspection of lazy tools. Keep startup graphs lazy while restoring the observable contracts at feature use. Lore-id: 7c51fd9a Confidence: high Scope-risk: wide Reversibility: reversible Tested: focused interactive, SDK, broker, pruning, memory, and workflow-gate suites (262 pass) Tested: W1c and W5b module traces Tested: bun run check:ts
Cached native bindings bypassed live identity adapters, while long-lived lock descriptors could retain stale bytes or report a transient Linux release mismatch. Resolve bindings at feature use, make descriptor writes complete and truncating, and allow only an independently verified exact-identity release fallback. Lore-id: 98bf5a24 Confidence: medium Scope-risk: wide Reversibility: reversible Tested: SDK broker lifecycle suites (75 pass) Not-tested: Linux 9,999-artifact migration locally; CI runner is authoritative
Discord and Slack share the exact unlink and process-incarnation lifecycle symbols moved behind lazy native bindings in this branch. Advance both serving generations and refresh the protected manifest so resident older daemons are replaced. Lore-id: 31cb749e Confidence: high Scope-risk: narrow Reversibility: reversible Tested: bun test packages/coding-agent/test/daemon-control.test.ts Tested: bun scripts/telegram-daemon-generation-guard.ts --validate-current-tree
Long artifact migrations can leave the retained Linux lock descriptor reporting a different inode even though the owner-only pathname still matches the original lock identity and attempt. Reopen that exact pathname without following links, verify it before and after release, and keep all other mismatches fail-closed. Lore-id: 4e9c0cb2 Confidence: medium Scope-risk: wide Reversibility: reversible Tested: managed lock lease and repaired regression suites (34 pass) Tested: bun run check:ts Not-tested: injected stale-descriptor recovery is Linux-only locally; affected CI covers it
The 9,999-artifact migration now completes and releases its lock, but Bun can return a one-shot EFAULT while recursively deleting that unusually large test tree. Retry only that transient cleanup code and still fail after three attempts. Lore-id: 0288db21 Confidence: medium Scope-risk: narrow Reversibility: reversible Tested: bun --cwd=packages/coding-agent run check Not-tested: Linux EFAULT retry locally; affected CI reproduces the boundary
…vailable Mid-run tool-output pruning must not report a successful prune when eviction artifacts cannot be established. Ephemeral install failure (or a null manager) with a non-empty prune plan now returns without committing partial non-tool prunes, so maintenance continues to compaction and fails closed. Update the regression fixture to force ephemeral install failure via SessionManagerTestHooks so it no longer relies on in-memory sessions silently getting a working tmpdir artifact store. Lore-id: midrun-fc1 Confidence: high Scope-risk: narrow Reversibility: reversible Tested: bun test packages/coding-agent/test/agent-session-midrun-maintenance.test.ts (18 pass) Not-tested: full coding-agent shard matrix
The live dev branch advanced Telegram owner postmortem authority while this PR carried lazy native daemon authority. Publish their union as generation 55, regenerate both manifests, and remove a duplicate prune fixture introduced while reconciling the concurrent session-maintenance changes. Lore-id: f1cd58a4 Confidence: high Scope-risk: wide Reversibility: migration-needed Tested: repaired regression suites (283 pass) Tested: daemon control and managed lock suites (165 pass) Tested: Telegram generation contracts and manifest guard Tested: W1c/W5b module traces and literal catalog gates
The live-base rebase replayed the provider-local relative import after the compatibility helper moved to the AI package root. Point it at the sibling core types module so publish and workspace type checks resolve again. Lore-id: 4b9b1fb0 Confidence: high Scope-risk: narrow Reversibility: easy Tested: coding-agent package check Tested: AI package check and provider-free core test
e69f956 to
a8851ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8851ab248
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| initialize: async () => { | ||
| switch (resolveMemoryBackendId(settings)) { |
There was a problem hiding this comment.
Reinitialize memory service when the backend setting changes
After session startup initializes this lazy service, changing memory.backend through the interactive settings selector cannot change its cached value. For example, a session started with local continues injecting and running the local backend after the user selects off, whereas the previous per-operation resolver observed the current setting. Reset or replace the service when this setting changes, or make the facade dispatch against the live backend selection.
Useful? React with 👍 / 👎.
| settingsInstance.get("colorBlindMode"), | ||
| settingsInstance.get("theme.dark"), | ||
| settingsInstance.get("theme.light"), | ||
| settingsInstance.get("syntaxHighlighting.enabled"), |
There was a problem hiding this comment.
Apply syntax-highlighting changes to the live theme
When an interactive user changes syntaxHighlighting.enabled in the settings selector, the new value is persisted but rendering does not change because the theme module copies it into syntaxHighlightingEnabledState only during this startup call, and the selector's change handler has no case that updates that state. Consequently the new appearance toggle has no effect until the CLI is restarted; wire the setting change to a live setter or theme reinitialization.
Useful? React with 👍 / 👎.
No description provided.